fix(wallet): Don't fail in build_fee_bump for missing parent txid
#337
Conversation
Pull Request Test Coverage Report for Build 19005356703Details
💛 - Coveralls |
| let fee_rate = self | ||
| .calculate_fee_rate(&tx) | ||
| .map_err(|_| BuildFeeBumpError::FeeRateUnavailable)?; | ||
| let fee_rate = fee / tx.weight(); |
There was a problem hiding this comment.
What's the rationale to not use calculate_fee_rate?
There was a problem hiding this comment.
It seemed redundant since calculate_fee_rate already calls calculate_fee, so I chose to inline the implementation here.
tests/add_foreign_utxo.rs
Outdated
| .script_pubkey(); | ||
| let witness_utxo = TxOut { | ||
| value: Amount::ZERO, | ||
| script_pubkey: ScriptBuf::new_p2a(), |
There was a problem hiding this comment.
new_p2a is not in rust-bitcoin 0.32.6, but in rust-bitcoin 0.32.7. Missing Cargo.toml update I guess
tests/add_foreign_utxo.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn test_add_foreign_utxo_bump_fee() { |
There was a problem hiding this comment.
I would mention the type of script used in the first tx input, as I think is important for discoverability and also to illustrate the context of what this test is doing.
tests/add_foreign_utxo.rs
Outdated
| let tx = psbt.unsigned_tx.clone(); | ||
| assert!(tx.input.iter().any(|txin| txin.previous_output == outpoint)); | ||
| let txid1 = tx.compute_txid(); | ||
| insert_tx(&mut wallet, tx); |
There was a problem hiding this comment.
A comment about the state in which this Tx is inserted from the perspective of the wallet would be helpful.
Like assume tx was broadcasted and is in mempool
| }; | ||
|
|
||
| let mut tx_builder = wallet.build_tx(); | ||
| tx_builder |
There was a problem hiding this comment.
The default fee rate of this tx is important but not expressed in the creation of the first tx of the test. A comment may be necessary here.
src/wallet/mod.rs
Outdated
| }) { | ||
| // This is a local utxo. | ||
| Some(&(keychain, derivation_index)) => { | ||
| let txout = prev_txout.ok_or(BuildFeeBumpError::UnknownUtxo(outpoint))?; |
There was a problem hiding this comment.
I think the error here is impossible to reach.
If the spk is present on the index, then this wallet collaborated on the creation of the transaction with the signature of that input, then that UTXO should have been already known by this wallet, right?
Also, we are getting this from the TxGraph itself and using an and_then call. All UnknownUtxos will be transformed into None and parsed by the other match arm before reaching this line.
There was a problem hiding this comment.
I will look for a way to rewrite it for clarity. I agree we should always have the txout, as otherwise we'd fail at calculate_fee.
ValuedMammal
force-pushed
the
fix/build_fee_bump_foreign_utxo
branch
from
f143e1f to
592217f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #337 +/- ##
==========================================
+ Coverage 84.81% 85.24% +0.43%
==========================================
Files 23 23
Lines 8145 8229 +84
==========================================
+ Hits 6908 7015 +107
+ Misses 1237 1214 -23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I see this PR is in the "Wallet 2.3.0" milestone but merging to the |
4 participants
Description
Previously
build_fee_bumpcould error withUnknownUtxoif a parent of the tx being fee-bumped wasn't found in the wallet. This made it impossible to usebuild_fee_bumpon a transaction created usingadd_foreign_utxo. This PR is a refactor ofbuild_fee_bumpthat manages to avoid the error by instead querying the tx graph for the txout specifically. This is a reasonable assumption because the previous txouts are necessary to compute the fee of the original tx.I've included a test in 6f214d5 to demonstrate the old behavior which now passes.
may resolve #325.
Notes to the reviewers
Changelog notice
TBD
Checklists
All Submissions:
New Features:
Bugfixes:
[ ] This pull request breaks the existing API